Conversation
chore: merge main draft-v31
## What ❔ <!-- What are the changes this PR brings about? --> <!-- Example: This PR adds a PR template to the repo. --> <!-- (For bigger PRs adding more context is appreciated) --> ## Why ❔ <!-- Why are these changes done? What goal do they contribute to? What are the principles behind them? --> <!-- The `Why` has to be clear to non-Matter Labs entities running their own ZK Chain --> <!-- Example: PR templates ensure PR reviewers, observers, and future iterators are in context about the evolution of repos. --> ## Is this a breaking change? - [ ] Yes - [ ] No ## Operational changes <!-- Any config changes? Any new flags? Any changes to any scripts? --> <!-- Please add anything that non-Matter Labs entities running their own ZK Chain may need to know --> ## Checklist <!-- Check your PR fulfills the following items. --> <!-- For draft PRs check the boxes as you complete them. --> - [ ] PR title corresponds to the body of PR (we generate changelog entries from PRs). - [ ] Tests for the changes have been added / updated. - [ ] Documentation comments have been added / updated. - [ ] Code has been formatted via `zkstack dev fmt` and `zkstack dev lint`.
## What ❔ <!-- What are the changes this PR brings about? --> <!-- Example: This PR adds a PR template to the repo. --> <!-- (For bigger PRs adding more context is appreciated) --> ## Why ❔ <!-- Why are these changes done? What goal do they contribute to? What are the principles behind them? --> <!-- The `Why` has to be clear to non-Matter Labs entities running their own ZK Chain --> <!-- Example: PR templates ensure PR reviewers, observers, and future iterators are in context about the evolution of repos. --> ## Is this a breaking change? - [ ] Yes - [ ] No ## Operational changes <!-- Any config changes? Any new flags? Any changes to any scripts? --> <!-- Please add anything that non-Matter Labs entities running their own ZK Chain may need to know --> ## Checklist <!-- Check your PR fulfills the following items. --> <!-- For draft PRs check the boxes as you complete them. --> - [ ] PR title corresponds to the body of PR (we generate changelog entries from PRs). - [ ] Tests for the changes have been added / updated. - [ ] Documentation comments have been added / updated. - [ ] Code has been formatted via `zkstack dev fmt` and `zkstack dev lint`.
## What ❔ <!-- What are the changes this PR brings about? --> <!-- Example: This PR adds a PR template to the repo. --> <!-- (For bigger PRs adding more context is appreciated) --> ## Why ❔ <!-- Why are these changes done? What goal do they contribute to? What are the principles behind them? --> <!-- The `Why` has to be clear to non-Matter Labs entities running their own ZK Chain --> <!-- Example: PR templates ensure PR reviewers, observers, and future iterators are in context about the evolution of repos. --> ## Is this a breaking change? - [ ] Yes - [ ] No ## Operational changes <!-- Any config changes? Any new flags? Any changes to any scripts? --> <!-- Please add anything that non-Matter Labs entities running their own ZK Chain may need to know --> ## Checklist <!-- Check your PR fulfills the following items. --> <!-- For draft PRs check the boxes as you complete them. --> - [ ] PR title corresponds to the body of PR (we generate changelog entries from PRs). - [ ] Tests for the changes have been added / updated. - [ ] Documentation comments have been added / updated. - [ ] Code has been formatted via `zkstack dev fmt` and `zkstack dev lint`.
## What ❔ The PR modified the tests to be more precise but also the main motivation for the refactor has been the fact that previously the balances of Gateway were tracked implicitly, which stopped working once chain balance stopped being tracking immediately upon interop ## Why ❔ <!-- Why are these changes done? What goal do they contribute to? What are the principles behind them? --> <!-- The `Why` has to be clear to non-Matter Labs entities running their own ZK Chain --> <!-- Example: PR templates ensure PR reviewers, observers, and future iterators are in context about the evolution of repos. --> ## Is this a breaking change? - [ ] Yes - [ ] No ## Operational changes <!-- Any config changes? Any new flags? Any changes to any scripts? --> <!-- Please add anything that non-Matter Labs entities running their own ZK Chain may need to know --> ## Checklist <!-- Check your PR fulfills the following items. --> <!-- For draft PRs check the boxes as you complete them. --> - [ ] PR title corresponds to the body of PR (we generate changelog entries from PRs). - [ ] Tests for the changes have been added / updated. - [ ] Documentation comments have been added / updated. - [ ] Code has been formatted via `zkstack dev fmt` and `zkstack dev lint`.
## What ❔ Reenables previously commented out tests and fixes them: - EN integration tests - Upgrade tests for the WITH_GATEWAY case <!-- What are the changes this PR brings about? --> <!-- Example: This PR adds a PR template to the repo. --> <!-- (For bigger PRs adding more context is appreciated) --> ## Why ❔ <!-- Why are these changes done? What goal do they contribute to? What are the principles behind them? --> <!-- The `Why` has to be clear to non-Matter Labs entities running their own ZK Chain --> <!-- Example: PR templates ensure PR reviewers, observers, and future iterators are in context about the evolution of repos. --> ## Is this a breaking change? - [ ] Yes - [ ] No ## Operational changes <!-- Any config changes? Any new flags? Any changes to any scripts? --> <!-- Please add anything that non-Matter Labs entities running their own ZK Chain may need to know --> ## Checklist <!-- Check your PR fulfills the following items. --> <!-- For draft PRs check the boxes as you complete them. --> - [ ] PR title corresponds to the body of PR (we generate changelog entries from PRs). - [ ] Tests for the changes have been added / updated. - [ ] Documentation comments have been added / updated. - [ ] Code has been formatted via `zkstack dev fmt` and `zkstack dev lint`.
## What ❔ <!-- What are the changes this PR brings about? --> <!-- Example: This PR adds a PR template to the repo. --> <!-- (For bigger PRs adding more context is appreciated) --> ## Why ❔ <!-- Why are these changes done? What goal do they contribute to? What are the principles behind them? --> <!-- The `Why` has to be clear to non-Matter Labs entities running their own ZK Chain --> <!-- Example: PR templates ensure PR reviewers, observers, and future iterators are in context about the evolution of repos. --> ## Is this a breaking change? - [ ] Yes - [ ] No ## Operational changes <!-- Any config changes? Any new flags? Any changes to any scripts? --> <!-- Please add anything that non-Matter Labs entities running their own ZK Chain may need to know --> ## Checklist <!-- Check your PR fulfills the following items. --> <!-- For draft PRs check the boxes as you complete them. --> - [ ] PR title corresponds to the body of PR (we generate changelog entries from PRs). - [ ] Tests for the changes have been added / updated. - [ ] Documentation comments have been added / updated. - [ ] Code has been formatted via `zkstack dev fmt` and `zkstack dev lint`.
|
To use Codex here, create a Codex account and connect to github. |
Detected VM performance changes
⚠ Detected differing instruction counts
Changes in number of opcodes executed indicate that the gas price of the benchmark has changed, which causes it to run out of gas at a different time. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c396d2917
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| match value { | ||
| 0 => Ok(L2DACommitmentScheme::None), | ||
| 1 => Ok(L2DACommitmentScheme::EmptyNoDA), | ||
| 2 => Ok(L2DACommitmentScheme::PubdataKeccak256), | ||
| 3 => Ok(L2DACommitmentScheme::BlobsAndPubdataKeccak256), | ||
| _ => Err("Invalid L2DACommitmentScheme value"), |
There was a problem hiding this comment.
Accept BlobsZksyncOS in DA scheme decoding
L2DACommitmentScheme declares BlobsZksyncOS = 4, but TryFrom<u8> still rejects 4 as invalid. This breaks chains that set the new scheme: get_zk_chain_on_chain_params() decodes on-chain DA params via try_from(...).expect(...), so receiving value 4 will panic instead of loading config. Add the missing 4 => Ok(L2DACommitmentScheme::BlobsZksyncOS) mapping (and keep parsers aligned).
Useful? React with 👍 / 👎.
| SettlementLayer::L1(SLChainId(settlement_layer_chain_id.unwrap_or(29) as u64)) | ||
| } | ||
| Some("Gateway") => { | ||
| SettlementLayer::Gateway(SLChainId(settlement_layer_chain_id.unwrap_or(506) as u64)) |
There was a problem hiding this comment.
Stop hardcoding fallback settlement chain IDs
For rows with NULL settlement_layer_chain_id, this conversion injects hardcoded IDs (29, 506, 19). The accompanying migration adds settlement_layer_chain_id without backfilling historical batches, so upgraded databases will decode existing headers with incorrect chain IDs. Since startup cursor construction reads the latest batch header to determine settlement layer, this can propagate a wrong settlement chain ID into post-restart processing. Use a deterministic source (stored value or node config), not baked-in constants.
Useful? React with 👍 / 👎.
| @@ -0,0 +1 @@ | |||
| ALTER TABLE miniblocks DROP COLUMN l2_da_commitment_scheme; | |||
There was a problem hiding this comment.
Reapply NOT NULL in DA schema rollback
The forward migration drops NOT NULL from miniblocks.l2_da_validator_address, but the down migration only drops l2_da_commitment_scheme. Rolling back therefore does not restore the original schema, leaving nullability looser than before and violating assumptions older code may rely on. The down migration should include the inverse ALTER COLUMN ... SET NOT NULL.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c396d2917
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 0 => Ok(L2DACommitmentScheme::None), | ||
| 1 => Ok(L2DACommitmentScheme::EmptyNoDA), | ||
| 2 => Ok(L2DACommitmentScheme::PubdataKeccak256), | ||
| 3 => Ok(L2DACommitmentScheme::BlobsAndPubdataKeccak256), | ||
| _ => Err("Invalid L2DACommitmentScheme value"), |
There was a problem hiding this comment.
Handle BlobsZksyncOS in L2DACommitmentScheme parsing
L2DACommitmentScheme defines BlobsZksyncOS = 4, but the numeric parser only accepts 0..=3, so any on-chain/config value of 4 is rejected as invalid. This makes the new scheme effectively unusable in code paths that decode from integers (e.g. DA pair reads) and can cascade into startup/runtime failures once a chain is configured with ZKsync OS DA commitments.
Useful? React with 👍 / 👎.
| .map(|a| L2DACommitmentScheme::try_from(*a as u8)) | ||
| .transpose() | ||
| .unwrap(), |
There was a problem hiding this comment.
Propagate DA scheme decode errors in consensus parser
ProtoRepr::read currently calls .unwrap() after decoding l2_da_commitment_scheme, which turns malformed or forward-compat enum values into a panic instead of a recoverable decode error. Since this path processes consensus payload data, a single unexpected value can crash the node process rather than returning an anyhow error and rejecting the payload safely.
Useful? React with 👍 / 👎.
| @@ -0,0 +1 @@ | |||
| ALTER TABLE miniblocks DROP COLUMN l2_da_commitment_scheme; | |||
There was a problem hiding this comment.
Reapply NOT NULL in DA commitment rollback migration
The up migration drops the NOT NULL constraint on miniblocks.l2_da_validator_address, but the down migration only removes l2_da_commitment_scheme and never restores the original constraint. Rolling this migration back leaves the schema less strict than before, which can break assumptions in older code that treated l2_da_validator_address as always present.
Useful? React with 👍 / 👎.
|
@codex review the whole PR |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c396d2917
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| self.l2_da_commitment_scheme | ||
| .as_ref() | ||
| .map(|a| L2DACommitmentScheme::try_from(*a as u8)) | ||
| .transpose() | ||
| .unwrap(), |
There was a problem hiding this comment.
Return parse errors instead of panicking on DA scheme
In ProtoRepr for proto::PubdataParams, invalid l2_da_commitment_scheme values currently hit .unwrap() and panic the process rather than rejecting the payload. Since this parser handles consensus payloads, a malformed or adversarial message can crash the node (DoS) instead of producing a normal decoding error path.
Useful? React with 👍 / 👎.
| 1 => Ok(SettlementLayer::Gateway(SLChainId( | ||
| *required(&self.chain_id).context("chain_id")?, | ||
| ))), | ||
| _ => unreachable!(), |
There was a problem hiding this comment.
Handle unknown settlement-layer tags without panic
The proto::SettlementLayer decoder uses unreachable!() for unknown enum tags, so a payload with an unexpected settlement_layer_type value will panic the node. Consensus/network input should be treated as untrusted and converted into a regular decoding error to avoid remotely triggerable crashes.
Useful? React with 👍 / 👎.
| unsealed_batch_header.interop_fee.as_u64() as i64, | ||
| unsealed_batch_header.pubdata_limit.map(|l| l as i64), | ||
| settlement_layer_type, | ||
| settlement_layer_chain_id as i32 |
There was a problem hiding this comment.
Preserve full settlement chain ID when inserting batches
insert_l1_batch_inner narrows settlement_layer_chain_id to i32 before inserting into l1_batches, which truncates valid larger chain IDs. For any settlement chain ID above 2_147_483_647, the stored value becomes incorrect, and subsequent reads will propagate the wrong settlement layer into execution / API behavior.
Useful? React with 👍 / 👎.
|
@codex review this pr/branch against |
|
To use Codex here, create a Codex account and connect to github. |
|
@codex review this pr against main |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c396d2917
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .get_latest_processed_interop_root_l1_batch_number() | ||
| let settlement_layer = self | ||
| .settlement_layer | ||
| .expect("settlement layer should be known"); |
There was a problem hiding this comment.
Handle missing settlement layer during migration
This helper now panics when self.settlement_layer is None, but EthTxAggregator is constructed from settlement_layer_for_sending_txs(), which is None specifically while migration is in progress. In that state gateway_status() resolves to InProgress and this path is executed, so the aggregator can crash exactly during migration instead of applying restrictions and continuing.
Useful? React with 👍 / 👎.
| let block = Some(BlockIdVariant::BlockNumber(BlockNumber::Pending)); | ||
| let response = main_node_client.call(request, block, None).await?; | ||
|
|
||
| if response.0.len() > 32 { |
There was a problem hiding this comment.
Reject empty interop fee call results
The decoder accepts any return length up to 32 bytes and left-pads it, so an empty eth_call response is treated as a valid fee of 0. If the target contract/function is unavailable on the main node (e.g. before upgrade), the periodic updater will overwrite the configured fallback fee with zero, causing underpriced gas estimation / execution parameters on the external node.
Useful? React with 👍 / 👎.
| .map(|a| L2DACommitmentScheme::try_from(*a as u8)) | ||
| .transpose() | ||
| .unwrap(), |
There was a problem hiding this comment.
Return decode error instead of panicking on bad DA scheme
l2_da_commitment_scheme is parsed with try_from(...) and then unwrapped, so an unknown / malformed enum value in consensus payloads will panic the process rather than returning a validation error. Since this parser handles serialized payload data, this creates a crash-on-input path instead of graceful rejection.
Useful? React with 👍 / 👎.
| 0x00, 0x01, 0x00, 0x11, | ||
| ]); | ||
|
|
||
| // todo FIXME, deploy normally instead using DUMMY_ADDRESS and deploying on genesis |
There was a problem hiding this comment.
please fix this todo before merging
|
|
||
| pub async fn get_l1_batch_interop_fee_if_sealed( | ||
| &mut self, | ||
| number: L1BatchNumber, | ||
| ) -> DalResult<Option<U256>> { | ||
| let row = sqlx::query!( | ||
| r#" | ||
| SELECT | ||
| interop_fee, | ||
| is_sealed | ||
| FROM | ||
| l1_batches | ||
| WHERE number = $1 | ||
| "#, | ||
| i64::from(number.0), | ||
| ) | ||
| .instrument("get_l1_batch_interop_fee_if_sealed") | ||
| .with_arg("number", &number) | ||
| .fetch_optional(self.storage) | ||
| .await?; | ||
|
|
||
| Ok(row.and_then(|row| row.is_sealed.then_some(U256::from(row.interop_fee as u64)))) | ||
| } |
There was a problem hiding this comment.
why you are not checking is_sealed in query?
| from_settlement_layer(settlement_layer); | ||
| let count = sqlx::query_scalar!( | ||
| r#" | ||
| SELECT COUNT(*) |
There was a problem hiding this comment.
you can use SELECT EXIST
|
|
||
| /// Returns `true` if there is any batch on the provided settlement layer that isn't fully committed yet. | ||
| /// This includes unsealed batches and sealed batches without a finalized commit tx. | ||
| pub async fn has_uncommitted_batches_on_settlement_layer( |
There was a problem hiding this comment.
todo for @Deniallugo check the correctness, seems we already have it
| assert_eq!(res.initial_storage_writes, basic_initial_writes); | ||
| assert_eq!(res.repeated_storage_writes, 2); |
| ); | ||
|
|
||
| assert_eq!( | ||
| assert_ne!( |
| // Proof-based interop on Gateway, meaning the Merkle proof hashes to Gateway's MessageRoot | ||
| ProofBasedGateway, | ||
| // Proof-based interop on L1, meaning the Merkle proof hashes to L1's MessageRoot | ||
| // ProofBasedL1, // todo: v30 |
| BASE_TOKEN_HOLDER_ADDRESS, | ||
| ContractLanguage::Sol, | ||
| ), | ||
| // todo FIXME, deploy normally instead using DUMMY_ADDRESS and deploying on genesis |
| } else { | ||
| connection | ||
| .blocks_dal() | ||
| .get_l1_batch_interop_fee_if_sealed(resolved_block_info.vm_l1_batch_number) | ||
| .await | ||
| .with_context(|| { | ||
| format!( | ||
| "failed loading interop fee for L1 batch #{}", | ||
| resolved_block_info.vm_l1_batch_number | ||
| ) | ||
| })? | ||
| .unwrap_or_default() | ||
| } | ||
| } else { | ||
| connection | ||
| .blocks_dal() | ||
| .get_l1_batch_interop_fee_if_sealed(resolved_block_info.vm_l1_batch_number) | ||
| .await | ||
| .with_context(|| { | ||
| format!( | ||
| "failed loading interop fee for L1 batch #{}", | ||
| resolved_block_info.vm_l1_batch_number | ||
| ) | ||
| })? | ||
| .or_else(|| self.interop_fee_fallback()) | ||
| .unwrap_or_default() |
|
@codex review the zkstack_cli folder changes in this PR against main |
What ❔
Why ❔
Is this a breaking change?
Operational changes
Checklist
zkstack dev fmtandzkstack dev lint.